Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

asl · 2024-11-01T12:47:14Z

Lots of cleanups and fixes here and there

asl · 2024-11-01T12:52:00Z

This is PR on top of #4971 for simplicity.

As mentioned in #4974, IR::Annotations incur a huge overhead, and in general does not make any sense: it's just another level of indirection. So, this just implements old FIXME, plus does some cleanup here and there.

The impact of # of allocations is huge.
Before:

After:

So, we're 10% less of memory allocations by both total size and number.

The runtime impact with GC disabled is huge as well:

Command	Mean [s]	Min [s]	Max [s]	Relative
`test/gtestp4c-nogc-main --gtest_filter=P4CParserUnroll.switch_20160512`	3.240 ± 0.048	3.195	3.422	1.10 ± 0.03
`test/gtestp4c-nogc --gtest_filter=P4CParserUnroll.switch_20160512`	2.948 ± 0.061	2.855	3.041	1.00

So, we're 10% faster here (!)

Runtime impact with GC on is much less visible due to huge GC overhead (1.5x):

Command	Mean [s]	Min [s]	Max [s]	Relative
`test/gtestp4c-main --gtest_filter=P4CParserUnroll.switch_20160512`	4.584 ± 0.099	4.312	4.897	1.02 ± 0.03
`test/gtestp4c --gtest_filter=P4CParserUnroll.switch_20160512`	4.512 ± 0.072	4.309	4.649	1.00

asl · 2024-11-01T13:18:19Z

sanitizers are broken after #4989

ChrisDodd · 2024-11-06T06:32:45Z

ir/ir.def

@@ -154,7 +157,7 @@ class P4Control : Type_Declaration, INestedNamespace, ISimpleNamespace, IApply,

 /// A P4-16 action
 class P4Action : Declaration, ISimpleNamespace, IAnnotated, IFunctional {
-    optional Annotations        annotations = Annotations::empty;


Would we get most/all of the memory savings by just using inline here instead of getting rid of IR::Annotations completely? That might be a simpler change.

Not all, as IR::Annotations will be still be a Node and it will still be cloned. Though "invisible" now when outer node is cloned by itself. Plus, there was already a FIXME for this change. I think additional wrapper is not required as annotation is a property of the Node itself, so why do we need to have additional level of indirection? For convenience we can certainly do using Annotations = IR::Vector<IR::Annotation> here, yes.

Plus I already fixed couple of bugs when it was Annotations (being a Node) queried for the presence of an Annotation. Certainly the check returned always false. Now only IAnnotated objects could be queried about annotations.

In addition to this: usually we only have 1-2 annotations max (e.g. @name and that's it). So we can store them inline using e.g. absl::InlineVector as underlying storage. This would remove almost all Annotation-related memory allocations. But first we'd need to reduce the size of Allocation itself IMO

I think additional wrapper is not required as annotation is a property of the Node itself, so why do we need to have additional level of indirection?

I agree.

asl · 2024-11-06T09:05:53Z

I rebased the branch after abseil string helpers were merged in. Now it should show the changes more cleanly

Signed-off-by: Anton Korobeynikov <[email protected]>

initializer_list while there Signed-off-by: Anton Korobeynikov <[email protected]>

Signed-off-by: Anton Korobeynikov <[email protected]>

Lots of cleanups and fixes here and there Signed-off-by: Anton Korobeynikov <[email protected]>

vlstill

Looks good, thank you. I went over ir, midend and most of frontend changes (except mainly of P4-14).

vlstill · 2024-11-07T06:06:25Z

ir/annotations.cpp

+    annotations.push_back(ann);
+}
+
+Vector<Annotation> addNameAnnotation(cstring name, const Vector<Annotation> &annos) {


Any reason to put the annotation vector second in this function, while it is first in the previous once?

vlstill · 2024-11-07T06:13:45Z

ir/base.def

+    bool needsParsing : 1;
+
+    /// If this is true this is a structured annotation, and there are some
+    /// constraints on its contents.
+    bool structured : 1;


Does the bit-field really decrease size of the struct? Since there are the Vector/IndexedVector members, I would expect the alignment to be 64bit and therefore it would not really meatter if the 2 bools are 2 bits or 2 bytes. Or is there another reason to use bit-field?

vlstill · 2024-11-07T06:17:56Z

ir/ir.def

@@ -154,7 +157,7 @@ class P4Control : Type_Declaration, INestedNamespace, ISimpleNamespace, IApply,

 /// A P4-16 action
 class P4Action : Declaration, ISimpleNamespace, IAnnotated, IFunctional {
-    optional Annotations        annotations = Annotations::empty;


I think additional wrapper is not required as annotation is a property of the Node itself, so why do we need to have additional level of indirection?

I agree.

vlstill · 2024-11-07T06:22:28Z

midend/flattenInterfaceStructs.h

@@ -72,10 +73,10 @@ struct StructTypeReplacement : public IHasDbPrint {
        flatten(typeMap, cstring::empty, type, type->annotations, vec, policy);
        if (type->is<IR::Type_Struct>()) {
            replacementType =
-                new IR::Type_Struct(type->srcInfo, type->name, IR::Annotations::empty, *vec);
+                new IR::Type_Struct(type->srcInfo, type->name, IR::Vector<IR::Annotation>(), *vec);


Is the annotations list not optional in this case?

vlstill · 2024-11-07T06:31:12Z

frontends/p4/sideEffects.h

+        if (const auto *em = mi->to<P4::ExternMethod>())
+            return !em->method->hasAnnotation(IR::Annotation::noSideEffectsAnnotation);
+        if (const auto *ef = mi->to<P4::ExternFunction>())
+            return !ef->method->hasAnnotation(IR::Annotation::noSideEffectsAnnotation);


You can merge these by using ExternCall.

asl requested review from vlstill, ChrisDodd and fruffy November 1, 2024 12:47

asl force-pushed the annotations-clones branch from 497afd2 to aed00f8 Compare November 1, 2024 12:53

ChrisDodd reviewed Nov 6, 2024

View reviewed changes

asl force-pushed the annotations-clones branch from aed00f8 to 4a82acc Compare November 6, 2024 09:03

asl added 4 commits November 6, 2024 21:29

Simplify annotations query

6abc7ba

Signed-off-by: Anton Korobeynikov <[email protected]>

Rearrange the fields to reduce the size of object. Fix broken

ae1aafc

initializer_list while there Signed-off-by: Anton Korobeynikov <[email protected]>

Reduce the overheads

450945d

Signed-off-by: Anton Korobeynikov <[email protected]>

Remove IR::Annotations and make IAnnotated to carry annotations inline.

fa34687

Lots of cleanups and fixes here and there Signed-off-by: Anton Korobeynikov <[email protected]>

asl force-pushed the annotations-clones branch from 4a82acc to fa34687 Compare November 7, 2024 05:29

vlstill reviewed Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

asl commented Nov 1, 2024

asl commented Nov 1, 2024 •

edited

Loading

asl commented Nov 1, 2024

ChrisDodd Nov 6, 2024 •

edited

Loading

asl Nov 6, 2024 •

edited

Loading

vlstill Nov 7, 2024

asl commented Nov 6, 2024

vlstill left a comment

vlstill Nov 7, 2024

vlstill Nov 7, 2024

vlstill Nov 7, 2024

vlstill Nov 7, 2024

vlstill Nov 7, 2024

Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

Are you sure you want to change the base?

Remove IR::Annotations and make IAnnotated to carry annotations inline #4992

Conversation

asl commented Nov 1, 2024

asl commented Nov 1, 2024 • edited Loading

asl commented Nov 1, 2024

ChrisDodd Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

asl Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

vlstill Nov 7, 2024

Choose a reason for hiding this comment

asl commented Nov 6, 2024

vlstill left a comment

Choose a reason for hiding this comment

vlstill Nov 7, 2024

Choose a reason for hiding this comment

vlstill Nov 7, 2024

Choose a reason for hiding this comment

vlstill Nov 7, 2024

Choose a reason for hiding this comment

vlstill Nov 7, 2024

Choose a reason for hiding this comment

vlstill Nov 7, 2024

Choose a reason for hiding this comment

asl commented Nov 1, 2024 •

edited

Loading

ChrisDodd Nov 6, 2024 •

edited

Loading

asl Nov 6, 2024 •

edited

Loading